Minimal effective gas price in the queue #8934
Conversation
tomusdrw
added
A0-pleasereview 🤓
transaction-pool/src/pool.rs
Outdated
| return None | ||
| } | ||
|
|
||
| self.worst_transactions.iter().next().map(|x| x.score.clone()) |
There was a problem hiding this comment.
Using next() here which looks correct (worst_transactions is ordered by ascending score). However I noticed: https://github.com/paritytech/parity/blob/0335e927e5d2133ceba4b54579094f1b4d810eac/transaction-pool/src/pool.rs#L277
Is the next_back() wrong then? Seems like it would check for the least worst transaction to replace.
There was a problem hiding this comment.
Great catch, I'll prepare a test and fix that issue with remove_worst
There was a problem hiding this comment.
Actually it's correct in remove_worst. ScoreWithRef has a custom Ord implementation that is ascending if the internal score is descending. So the error is actually here, will make sure to add a correct test to cover this.
There was a problem hiding this comment.
Looks good. Should this one also change to next_back() then?
There was a problem hiding this comment.
Indeed, preparing a test case to cover that and a fix.
|
Needs a 2nd review. |
tomusdrw
added
A4-gotissues 💥
tomusdrw
added
A0-pleasereview 🤓
|
@ascjones thanks for the review, the issue with |
|
Code looks fine, but I think this can be better encapsulated in scoring. Instead of passing the gas price into the verifier, could the queue just check The current implementation also has the effect of potentially rejecting txs even if the queue is not full (but at 90% capacity), which I'm not sure is intended/desirable. EDIT: I see using |
|
@jimpo thanks for the review, I've updated the PR a bit:
Unfortunately we are not able to re-use That said it's not really that much of an issue, the pool decisions are always local and are not part of the consensus itself. To have the most accurate image of the pending transactions one should run with big enough pool to store them all. |
5chdn
added
A7-looksgoodtestsfail 🤖
|
Please rebase on latest master to fix builds. |
5chdn
added
A8-looksgood 🦄
* Minimal effective gas price. * Fix naming, add test * Fix minimal entry score and add test. * Fix worst_transaction. * Remove effective gas price threshold. * Don't leak gas_price decisions out of Scoring.
* parity-version: bump beta to 1.11.6 * scripts: remove md5 checksums (#8884) * Add support for --chain tobalaba * Convert indents to tabs :) * Fixes for misbehavior reporting in AuthorityRound (#8998) * aura: only report after checking for repeated skipped primaries * aura: refactor duplicate code for getting epoch validator set * aura: verify_external: report on validator set contract instance * aura: use correct validator set epoch number when reporting * aura: use epoch set when verifying blocks * aura: report skipped primaries when generating seal * aura: handle immediate transitions * aura: don't report skipped steps from genesis to first block * aura: fix reporting test * aura: refactor duplicate code to handle immediate_transitions * aura: let reporting fail on verify_block_basic * aura: add comment about possible failure of reporting * Only return error log for rustls (#9025) * Transaction Pool improvements (#8470) * Don't use ethereum_types in transaction pool. * Hide internal insertion_id. * Fix tests. * Review grumbles. * Improve should_replace on NonceAndGasPrice (#8980) * Additional tests for NonceAndGasPrice::should_replace. * Fix should_replace in the distinct sender case. * Use natural priority ordering to simplify should_replace. * Minimal effective gas price in the queue (#8934) * Minimal effective gas price. * Fix naming, add test * Fix minimal entry score and add test. * Fix worst_transaction. * Remove effective gas price threshold. * Don't leak gas_price decisions out of Scoring. * Never drop local transactions from different senders. (#9002) * Recently rejected cache for transaction queue (#9005) * Store recently rejected transactions. * Don't cache AlreadyImported rejections. * Make the size of transaction verification queue dependent on pool size. * Add a test for recently rejected. * Fix logging for recently rejected. * Make rejection cache smaller. * obsolete test removed * obsolete test removed * Construct cache with_capacity. * Optimize pending transactions filter (#9026) * rpc: return unordered transactions in pending transactions filter * ethcore: use LruCache for nonce cache Only clear the nonce cache when a block is retracted * Revert "ethcore: use LruCache for nonce cache" This reverts commit b382c19. * Use only cached nonces when computing pending hashes. * Give filters their own locks, so that they don't block one another. * Fix pending transaction count if not sealing. * Clear cache only when block is enacted. * Fix RPC tests. * Address review comments. * A last bunch of txqueue performance optimizations (#9024) * Clear cache only when block is enacted. * Add tracing for cull. * Cull split. * Cull after creating pending block. * Add constant, remove sync::read tracing. * Reset debug. * Remove excessive tracing. * Use struct for NonceCache. * Fix build * Remove warnings. * Fix build again. * miner: add missing macro use for trace_time * ci: remove md5 merge leftovers
…rp_sync_on_light_client * 'master' of https://github.com/paritytech/parity: Attempt to graceful shutdown in case of panics (openethereum#8999) simplify kvdb error types (openethereum#8924) Add option for user to set max size limit for RPC requests (openethereum#9010) bump ntp to 0.5.0 (openethereum#9009) Removed duplicate dependency (openethereum#9021) Minimal effective gas price in the queue (openethereum#8934)
For small (and saturated) queues this optimizes the verification pipeline to avoid recovering a sender in case we are quite sure that the transaction won't get into the pool anyway.